drivers/, dummy-ups: modernize status/alarm handling, ALARM instruction for dummy modes #2936

desertwitch · 2025-05-03T08:51:11Z

follow up to #2934
fixes #2937
implements ideas of #2928, #2931

modernizes remaining 4 drivers to use the contemporary best-practice status/alarm handling:

 - `clone`, `clone-outlet`, `nhs_ser`, `nutdrv_qx_ablerex` driver updates:
   * Refactored to follow modern handling of status and alarm conditions,
     aligning with current driver design practices. This includes fixing
     copy-paste related issues in alarm reporting and removing some alarm
     messages that should instead be reflected as status flags.

introduces an ALARM instruction for dummy modes in dummy-ups to allow simulating modern alarms:

 - `dummy-ups` driver updates:
   * A new instruction `ALARM` was added for the `Dummy Mode` operation
     of the driver, enabling simulation of UPS alarm states more closely
     in line with modern, real-world UPS driver implementations. This
     follows the updated principle of keeping alarm states decoupled from
     the `ups.status` variable, with alarms now raised via common alarm
     functions rather than direct manipulation.

Another, more recently introduced instruction is `ALARM`, which allows
the simulation of UPS alarms in much the same way they would be raised
by real driver implementations. Modern drivers decouple alarm states
from the `ups.status` variable, and raising `ALARM` by setting it as a
status token is discouraged in favor of using modern, common functions
for raising alarms within the driver code.

The `ALARM` instruction is intended to simulate this behavior as closely
as possible. The value following an `ALARM` instruction is treated as
the alarm message and is eventually published as the `ups.alarm`
variable, with the `ALARM` token also set in `ups.status` by internal
driver mechanisms, rather than by directly manipulating the variable.
Multiple `ALARM` instructions will have their messages combined into
the `ups.alarm` variable, just as would happen with real driver logic.

Conversely, any `ALARM` instruction on its own will clear active alarm
states. See below for an example of setting and resetting alarms:

	ALARM [UPS too warm to charge]
	ALARM [UPS circuit is overheating]
	TIMER 5
	ALARM
	TIMER 5
	ALARM [UPS too cold to charge]

…RM instruction for dummy modes Signed-off-by: desertwitch <[email protected]>

Signed-off-by: desertwitch <[email protected]>

…dling changes Signed-off-by: desertwitch <[email protected]>

Signed-off-by: desertwitch <[email protected]>

desertwitch · 2025-05-03T13:37:31Z

Had some more time and figured this would also fit into this PR (and save on CI cycles), fixes #2937.
This PR should now conclude the effort to modernize all drivers to use status_* and alarm_* functions.

… outlets No longer evaluate outlet status in instant command handling, but rather in the main update cycle. Raising OFF for a turned-off outlet aligns with the behavior of the clone-outlet driver. Signed-off-by: desertwitch <[email protected]>

…ges [networkupstools#2936] Signed-off-by: desertwitch <[email protected]>

…etworkupstools#2936] Signed-off-by: desertwitch <[email protected]>

…ous work [networkupstools#2936] Signed-off-by: desertwitch <[email protected]>

jimklimov

Looks reasonable, but there's a lot of change in clone driver that I couldn't quite wrap my head around. Did you have some chance to test that it still works (no worse than before)? :)

jimklimov · 2025-05-10T21:32:27Z

drivers/clone-outlet.c

+	status_init();
+
 	if (outlet.status == 0) {
 		upsdebugx(2, "OFF flag set (%s: switched off)", prefix.status);
-		dstate_setinfo("ups.status", "%s OFF", ups.status);
-		return;
+		status_set("OFF");
 	}

 	if ((outlet.timer.shutdown > -1) && (outlet.timer.shutdown <= outlet.delay.shutdown)) {
 		upsdebugx(2, "FSD flag set (%s: -1 < [%ld] <= %ld)", prefix.timer.shutdown, outlet.timer.shutdown, outlet.delay.shutdown);
-		dstate_setinfo("ups.status", "FSD %s", ups.status);
-		return;
+		status_set("FSD");
 	}

-	upsdebugx(3, "%s: power state not critical", getval("prefix"));
-	dstate_setinfo("ups.status", "%s", ups.status);
+	status_set(ups.status); /* FIXME: Split token words? */
+	status_commit();


Just noting, with no judgement, that this also subtly changes the logic: previously the driver applied either "OFF" or "FSD" to existing ups.status and returned; now it can apply both.

It also no longer reports that the "power state is not critical" in debug log, though that may be not too useful as such.

Intended because the user should see all actual statuses and OFF/FSD should not be mutually exclusive, I did drop the debug message - sorry - but I think it made no sense when a UPS may well be OB LB or OVER even if not OFF or FSD yet, which would also be somewhat critical.

I did also pave it over for ease of coding around it, other than the argument above, truth be told, since all statuses (also critical ones) are reported summarily now.

desertwitch · 2025-05-11T04:35:23Z

Looks reasonable, but there's a lot of change in clone driver that I couldn't quite wrap my head around. Did you have some chance to test that it still works (no worse than before)? :)

Yes, all thinkable scenarios were tested by me and the driver works as before.

It may look like many changes but it's mostly moving things around to reduce redundant expensive checks via dstate to locations where we can also check it from the data we have. The same applies to setting statuses from deeply nested parsing functions, which was refactored into setting internal state instead and then updating from that in a central location in the update loop.

I think the only behavioral change in clone really was also setting OFF when an outlet is dead to make this consistent with clone-outlet and obvious to the user that the outlet is not giving power.

As far clone-outlet, the driver did just set FSD or OFF and return before, but that basically exposed nothing else from the UPS statuses and may have left the user wondering what other condition the UPS was in. I figured for completeness there'd be no harm giving them a full picture of the UPS statuses while also raising those two even as possible combinations of each other as they are not mutually exclusive.

Happy to answer any questions!

desertwitch · 2025-05-11T05:17:53Z

drivers/clone.c

+	struct {
+		char	*off;
+		char	*on;
+		char	*status;


Stores the user set arguments for re-use, instead of getvaling them all over

desertwitch · 2025-05-11T05:19:39Z

drivers/clone.c

+
+		if (ups.load.status && !strcasecmp(arg[1], ups.load.status)) {
+			if (!strcasecmp(arg[2], "off") || !strcasecmp(arg[2], "no")) {
+				outlet = 0;


Sets internal state from data we have, for later updating from directly in upsdrv_updateinfo(), instead of then needing to dstate_getinfo it again otherwhere to the same effect.

desertwitch · 2025-05-11T05:22:39Z

drivers/clone.c

-	if (val) {
-		if (!strcasecmp(val, "off") || !strcasecmp(val, "no")) {
-			outlet = 0;
-		}


Moved to parse_args where we have this information, if the outlet is off should not be set inside an instant command handling function, but where the data actually comes in and is parsed. This also obsoletes the need for the redundant dstate check which is expensive.

desertwitch · 2025-05-11T05:23:41Z

drivers/clone.c


+	ups.load.off = getval("load.off");
+	ups.load.on = getval("load.on");
+	ups.load.status = getval("load.status");


Record the user given arguments into the internal state struct, reduces needs for constant getvaling them within update loops.

desertwitch · 2025-05-11T05:24:59Z

drivers/clone.c

+		if (ups.load.status && !reported_off) { /* first entrance */
+			reported_off = 1;
+			upslogx(LOG_WARNING, "%s: upstream reports outlet off (setting OFF)",
+				__func__);


Inform user that the outlet is reported off and set OFF to make it consistent with clone-outlet, the user can see on a first glance the outlet is not giving power.

desertwitch · 2025-05-11T05:34:37Z

drivers/clone.c

 			}
+
+			if (!ups.load.status) {
+				/* Better than nothing if no load.status argument was given,


If no argument was given where to check the outlet's actual status, we assume it to have successfully turned off after sending ups.load.off for lack of a variable for checking. If an argument was given, no assumptions are made and the actual status of the outlet is checked.

Same as before, but it was just set here and overridden later in an unexpected place instcmd, this way should be cleaner and more traceable with debug messages.

desertwitch · 2025-05-11T05:35:52Z

drivers/clone.c


-			dstate_setinfo("ups.status", "%s", ups.status);
+			if (!ups.load.status) {
+				/* Better than nothing if no load.status argument was given,


Same as above: assume it to be on when the command was sent, if no argument for checking the status was given. Follows behaviour as before, just more traceable.

jimklimov · 2025-05-11T14:29:03Z

Super, thanks for the analyses!

desertwitch added 5 commits May 3, 2025 10:37

drivers/dummy-ups.c, docs/man/dummy-ups.txt, NEWS.adoc: introduce ALA…

6ddc788

…RM instruction for dummy modes Signed-off-by: desertwitch <[email protected]>

drivers/nhs_ser.c: modernize status and alarm handling

d254508

Signed-off-by: desertwitch <[email protected]>

drivers/nutdrv_qx_ablerex.c: modernize alarm handling

c42deaa

Signed-off-by: desertwitch <[email protected]>

NEWS.adoc: add nhs_ser, nutdrv_qx_ablerex modern alarm and status han…

6e87c75

…dling changes Signed-off-by: desertwitch <[email protected]>

NEWS.adoc: fill in pull request numbers for recent changes

057d8e4

Signed-off-by: desertwitch <[email protected]>

desertwitch mentioned this pull request May 3, 2025

clone.c, clone-outlet.c: UPS status handling needs modernization #2937

Closed

desertwitch marked this pull request as ready for review May 3, 2025 09:06

drivers/clone{,-outlet}.c, NEWS.adoc: modernize status handling

2a8f1ee

Signed-off-by: desertwitch <[email protected]>

desertwitch changed the title ~~drivers/, dummy-ups: modernize alarm handling, ALARM instruction for dummy modes~~ drivers/, dummy-ups: modernize status/alarm handling, ALARM instruction for dummy modes May 3, 2025

drivers/clone-outlet.c: fix whitespace [networkupstools#2936]

07ef33d

Signed-off-by: desertwitch <[email protected]>

desertwitch added 4 commits May 3, 2025 21:24

drivers/clone.c: further improve outlet status detection, debug messa…

aea45dd

…ges [networkupstools#2936] Signed-off-by: desertwitch <[email protected]>

drivers/clone.c: reduce redundant argument parsing, debug messages [n…

fc7d778

…etworkupstools#2936] Signed-off-by: desertwitch <[email protected]>

drivers/clone.c: refactor core driver logic, minor fixes around previ…

b5df41d

…ous work [networkupstools#2936] Signed-off-by: desertwitch <[email protected]>

jimklimov added enhancement refactor/fightwarn PR or issue proposal to improve code maintainability without functional changes, or to fix warnings NUT protocols impacts-release-2.8.3 Issues reported against NUT release 2.8.3 (maybe vanilla or with minor packaging tweaks) labels May 9, 2025

jimklimov added this to the 2.8.4 milestone May 9, 2025

Merge branch 'master' into modernize-drv-alarms

6c5631d

jimklimov approved these changes May 11, 2025

View reviewed changes

desertwitch commented May 11, 2025

View reviewed changes

Merge branch 'master' into modernize-drv-alarms

995a82c

jimklimov merged commit 0545c6e into networkupstools:master May 11, 2025
20 of 21 checks passed

desertwitch deleted the modernize-drv-alarms branch May 26, 2025 08:51

Uh oh!

drivers/, dummy-ups: modernize status/alarm handling, ALARM instruction for dummy modes #2936

drivers/, dummy-ups: modernize status/alarm handling, ALARM instruction for dummy modes #2936

Uh oh!

Conversation

desertwitch commented May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

desertwitch commented May 3, 2025

Uh oh!

jimklimov left a comment

Choose a reason for hiding this comment

Uh oh!

jimklimov May 10, 2025

Choose a reason for hiding this comment

Uh oh!

desertwitch May 11, 2025

Choose a reason for hiding this comment

Uh oh!

desertwitch commented May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

desertwitch May 11, 2025

Choose a reason for hiding this comment

Uh oh!

desertwitch May 11, 2025

Choose a reason for hiding this comment

Uh oh!

desertwitch May 11, 2025

Choose a reason for hiding this comment

Uh oh!

desertwitch May 11, 2025

Choose a reason for hiding this comment

Uh oh!

desertwitch May 11, 2025

Choose a reason for hiding this comment

Uh oh!

desertwitch May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

desertwitch May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimklimov commented May 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

desertwitch commented May 3, 2025 •

edited

Loading

desertwitch commented May 11, 2025 •

edited

Loading

desertwitch May 11, 2025 •

edited

Loading

desertwitch May 11, 2025 •

edited

Loading